Skip to content

Refactor config layer#71

Merged
JeanLucPons merged 12 commits intomainfrom
refactor_config
Nov 17, 2025
Merged

Refactor config layer#71
JeanLucPons merged 12 commits intomainfrom
refactor_config

Conversation

@TeresiaOlsson
Copy link
Copy Markdown
Contributor

@TeresiaOlsson TeresiaOlsson commented Nov 13, 2025

I'm attempting to refactor the configuration layer based on my suggestion in the aml repository and the comments from the maintainers meeting.

In the first step:

  • I have removed the PyAML class and moved everything related to loading from a file into the fileloader module.
  • I removed the dictionary of instrument since it's not required since it's easier to just load each instrument separately. Especially if people want to initialize all devices when loading and be able to keep the configuration for different instruments at different root paths.
  • I tried to add a similar interface as in pyAT where there are two options for creating a lattice, load_lattice or create a Lattice object directly. In this suggestion there is now load_instrument to load from file or you can create an Instrument object directly.

This aim to solve issue #28.

@TeresiaOlsson TeresiaOlsson marked this pull request as draft November 13, 2025 14:46
@TeresiaOlsson
Copy link
Copy Markdown
Contributor Author

I didn't manage to get it to work to do def load_instrument(filename:str, paths_stack:list=None) -> Instrument: because adding from pyaml.instrument import Instrument caused circular imports of the factory module. Maybe someone has a suggestion for how to get around that?

@gubaidulinvadim
Copy link
Copy Markdown
Contributor

For your circular import, it is caused (from what I understand) by wanting to specify the type (Instrument). In some packages, like numpy, there's a special typing module to avoid this.

@TeresiaOlsson
Copy link
Copy Markdown
Contributor Author

TeresiaOlsson commented Nov 13, 2025

Yes, since type checking is everywhere else I think it should be here too since it's best practice to do it. I tried to use TYPE_CHECKING from typing and also annotations from __future__ in case it was a problem related to Python 3.10 but none of the combinations I tried worked. But could also be that I need to add it at many more places than I tried since it seems like the factory module is imported in several other modules that are imported by Instrument. I ran into problems in at least controlsystem.py and cfm_magnet.py but could be more places.

@JeanLucPons
Copy link
Copy Markdown
Contributor

JeanLucPons commented Nov 14, 2025

Personally, i would really prefer to be able to write:

from pyaml.instrument import Instrument
sr = Instrument.load("tests/config/sr.yaml")
print(sr)

It uses a static class method and is much more in an object oriented style and last but not least you hide this paths_stack internal parameters.

class Instrument(object):    
    """PyAML top level class"""

    @staticmethod
    def load(filename:str) -> "Instrument":
        return load_instrument(filename)
...

The above works perfectly in vscode and provides completion.

@JeanLucPons
Copy link
Copy Markdown
Contributor

For solving this kind of circular import you can:
But you still need to use type annotation as str.

from typing import Union, TYPE_CHECKING

if TYPE_CHECKING:
    from pyaml.instrument import Instrument

def load_instrument(filename:str, paths_stack:list=None) -> "Instrument":

@TeresiaOlsson
Copy link
Copy Markdown
Contributor Author

For solving this kind of circular import you can: But you still need to use type annotation as str.

from typing import Union, TYPE_CHECKING

if TYPE_CHECKING:
    from pyaml.instrument import Instrument

def load_instrument(filename:str, paths_stack:list=None) -> "Instrument":

I will try it. Maybe I was then just missing the quotation marks for it to work.

@TeresiaOlsson
Copy link
Copy Markdown
Contributor Author

I have added the suggestions from @JeanLucPons. I will now rewrite the tests and then set it ready for review so you can review if it should be merged or not.

@JeanLucPons
Copy link
Copy Markdown
Contributor

JeanLucPons commented Nov 14, 2025

Personally I have no objection to merge this PR. As it will break backward compatibility with the frontend, the release number will be increased following #36. So if you want to rename Instrument to Accelerator and if nodoby has objection, please proceed.
doc and jupyter code from @simoneliuzzo will also have to be updated.

@simoneliuzzo
Copy link
Copy Markdown
Contributor

Also for me the single instrument (or Accelerator, ok for me as well) per yaml config file is also ok. I can help if needed, let me know @TeresiaOlsson

@TeresiaOlsson
Copy link
Copy Markdown
Contributor Author

Okay, then I will also add the changes to change the name from Instrument to Accelerator. My plan was to put that in another PR but then I will add it to this too.

@TeresiaOlsson TeresiaOlsson marked this pull request as ready for review November 17, 2025 14:17
@TeresiaOlsson
Copy link
Copy Markdown
Contributor Author

I have fixed the tests now and made it ready for review.

Copy link
Copy Markdown
Contributor

@JeanLucPons JeanLucPons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me.
Nice job @TeresiaOlsson !
Thanks

@JeanLucPons JeanLucPons merged commit e9ea0d9 into main Nov 17, 2025
2 checks passed
@gubaidulinvadim gubaidulinvadim deleted the refactor_config branch November 25, 2025 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants